Skip to content

use u8 for field array offset instead of usize #495

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Conversation

burrbull
Copy link
Member

No description provided.

@burrbull burrbull requested a review from a team as a code owner March 27, 2021 13:06
@rust-highfive
Copy link

r? @therealprof

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools labels Mar 27, 2021
@burrbull
Copy link
Member Author

cc @adamgreig

@adamgreig
Copy link
Member

Is the idea that field arrays can never have >255 elements because registers are only 32 (or maybe 64) bits wide, so there's no point using a large usize when a u8 will always work?

@therealprof
Copy link
Contributor

I'd also like to see what the concrete benefits are. Switching from usize to something else might require a bit of casting during use which is annoying (and error prone).

@burrbull
Copy link
Member Author

Is the idea that field arrays can never have >255 elements because registers are only 32 (or maybe 64) bits wide, so there's no point using a large usize when a u8 will always work?

Yes, I forget this when implemented. As field arrays are not really arrays, I don't think someone will use big usize accessing them.

@adamgreig
Copy link
Member

I'd also like to see what the concrete benefits are. Switching from usize to something else might require a bit of casting during use which is annoying (and error prone).

It's only used in bit shifting arithmetic, not as a slice index; this PR changes the generated code from something like:

            pub unsafe fn pin(&self, n: usize) -> PIN_R {
                PIN_R::new(((self.bits >> n) & 0x01) != 0)
            }

to something like:

            pub unsafe fn pin(&self, n: u8) -> PIN_R {
                PIN_R::new(((self.bits >> n) & 0x01) != 0)
            }

where in this case self.bits is 32-bit (or in general is the register width).

@burrbull, do any of the tests in the CI actually use field arrays?

@therealprof
Copy link
Contributor

It's only used in bit shifting arithmetic, not as a slice index; this PR changes the generated code from something like:

Fair enough, but in that case it's also unlikely to provide any benefit.

@adamgreig
Copy link
Member

We haven't actually shipped an svd2rust since field array support was added in #400, so I see this as more cleaning up the API for it before release. But, yes, I don't know if there's any benefit to swapping to u8 for this. If anything maybe it should use the same type as the registers to make it line up with self.pins etc?

@burrbull
Copy link
Member Author

do any of the tests in the CI actually use field arrays?

risk-v k210 should use them, but I'm not sure is test contain actual version of SVD.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants